Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mbedtls fixes 0.12 #135

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Mbedtls fixes 0.12 #135

merged 1 commit into from
Jan 18, 2024

Conversation

billatarm
Copy link
Collaborator

@billatarm billatarm commented Jan 2, 2024 •
edited
Loading

  • Fix linking options against system provided mbedtls It was me using an ETOOOLD version of mbedtls. Just don't run configure on vendor'd mbedtls wqhen env vars set.
    • Probably add a CI test for this, something like rm vendor directory, set build flags and ensure build works. Maybe even leer into open files of the process using the bindings and ensure its loaded mbedtls.
  • Deduplicate parts of build.rs

billatarm
billatarm commented Jan 2, 2024
View reviewed changes
psa-crypto-sys/build.rs Outdated Show resolved Hide resolved
@gowthamsk-arm
Copy link
Contributor

Thanks for the PR @billatarm :)
There are some trivial CI failures. Can you fix them, please?

@billatarm
Copy link
Collaborator Author

Thanks for the PR @billatarm :) There are some trivial CI failures. Can you fix them, please?

weird I wonder why these didn't trigger locally.

@billatarm
Copy link
Collaborator Author

Thanks for the PR @billatarm :) There are some trivial CI failures. Can you fix them, please?

weird I wonder why these didn't trigger locally.

ahh features

@billatarm billatarm force-pushed the mbedtls-fixes-0.12 branch 4 times, most recently from 899e219 to 9da5f5a Compare January 3, 2024 17:29
@billatarm
Copy link
Collaborator Author

hrmm some weird memory issues now, I wonder if it's getting some wrong definitions for type sizes when building the shim. I think its my usage of MBEDTLS_CONFIG_FILE

@billatarm
Copy link
Collaborator Author

hrmm some weird memory issues now, I wonder if it's getting some wrong definitions for type sizes when building the shim. I think its my usage of MBEDTLS_CONFIG_FILE

Ahh fedora has an ancient mbedtls 2.28.5 and we need 3.0 minimum. Half the changes in this patch are because I didn't catch how old Fedora's package was. This greatly simplifies the patch.

@billatarm billatarm force-pushed the mbedtls-fixes-0.12 branch 3 times, most recently from 003a311 to 51feea4 Compare January 4, 2024 01:03
gowthamsk-arm
gowthamsk-arm previously approved these changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@gowthamsk-arm gowthamsk-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

tgonzalezorlandoarm
tgonzalezorlandoarm requested changes Jan 16, 2024
View reviewed changes
Copy link
Member

@tgonzalezorlandoarm tgonzalezorlandoarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nits, LGTM apart from that

psa-crypto-sys/build.rs Outdated Show resolved Hide resolved
psa-crypto-sys/build.rs Show resolved Hide resolved
@billatarm billatarm force-pushed the mbedtls-fixes-0.12 branch 3 times, most recently from 8ee9e8e to 393ad05 Compare January 17, 2024 15:23
@billatarm
psa-crypto-sys: dont configure mbedtls on external builds
f2b9062 
When setting env vars to use a external mbedTLS library, for example
provided by a system distro, like so:
MBEDTLS_INCLUDE_DIR="/usr/include/"
MBEDTLS_LIB_DIR="/usr/lib64"

The build would configure the vendored mbedtls. Fix this by only
configuring the vendored mbedtls when being used. This would allow
clones without the submodule using a system supplied mbedtls to succeed.

Signed-off-by: Bill Roberts <[email protected]>
tgonzalezorlandoarm
tgonzalezorlandoarm approved these changes Jan 17, 2024
View reviewed changes
Copy link
Member

@tgonzalezorlandoarm tgonzalezorlandoarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ionut-arm
ionut-arm approved these changes Jan 18, 2024
View reviewed changes
Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, looks sensible to me!

@tgonzalezorlandoarm tgonzalezorlandoarm merged commit ea0499c into parallaxsecond:main Jan 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gowthamsk-arm gowthamsk-arm gowthamsk-arm approved these changes

@tgonzalezorlandoarm tgonzalezorlandoarm tgonzalezorlandoarm approved these changes

@ionut-arm ionut-arm ionut-arm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@billatarm @gowthamsk-arm @ionut-arm @tgonzalezorlandoarm